Skip to content

Conversation

@RafaelKayumov
Copy link
Contributor

@RafaelKayumov RafaelKayumov commented Jul 3, 2025

Fix: Display destination address for purchased shipping labels

WOOMOB-746

Description

This pull request fixes a bug where the destination address for previously purchased shipping labels was not being displayed to the user.

The root cause was that the config/label-purchase/{order_id} endpoint returns shipping label objects and their corresponding destination addresses in separate parts of the JSON payload. The ShippingLabel model itself did not contain the address, and the app was not performing the necessary association between the two datasets.

This PR introduces a client-side solution to correctly populate the address information:

  1. Enhanced Response Parsing: The app's data models have been updated to correctly parse the selected_destination dictionary from the config.shippingLabelData.storedData path in the API response. The config.shippingLabelData.storedData.selected_destination request parameter was added to obtain the selected_destination payload.
  2. Client-Side Address Mapping: A new mapping mechanism has been implemented. After the config response is decoded, this logic iterates through the currentOrderLabels and injects the correct destination address from the selected_destination dictionary.
  3. Standardized Shipment ID: The address-to-label mapping relies on a standardized shipment_<id> format for the dictionary key. A new WooShippingShipmentIDFormatter utility was created to handle this formatting consistently. It was extracted from the recently added in for reusability WooShippingPackagePurchase.swift
  4. Backward Compatibility: To support labels created before this change (which may not use the new key format), the mapping logic includes a fallback that attempts to look up the destination address using the original, non-formatted numeric shipmentID.

Testing steps

  • Create a new order with a bunch of physical products. Make it completed.
  • Split the order into 2+ shipments.
  • Setup shipments with different hazmat, customs and destination country configurations. For example - the 1st shipment has non foreign destination address, no customs data, but has hazmat. The second shipment has foreign destination address and customs data.
  • Purchase labels for both shipments.
  • Close order details and then navigate back to this order from order list.
  • Expand bottom sheet to reveal destination address details.
  • Check out each fulfilled shipment you've just purchased.
  • Each shipment should present the correct destination address.
  • Consider comparing with web behaviour. Web version should display same addresses for those shipments.

Testing information

This PR is thoroughly covered by new automated tests to ensure the mapping logic is correct and to prevent regressions.

  • WooShippingShipmentIDFormatterTests: A new test file was created to unit test the ID formatting logic, covering various input scenarios.
  • WooShippingRemoteTests: The existing remote tests were refactored to use the established waitFor and simulateResponse pattern. The new tests validate the end-to-end flow by mocking the loadConfig API call with different JSON fixtures:
    • wooshipping-config-with-destinations.json: Simulates a response containing destination addresses and asserts that they are correctly mapped to the label objects.
    • wooshipping-config-without-destinations.json: Ensures the app handles responses without destination data gracefully.
    • wooshipping-config-with-invalid-destinations.json: Confirms that a malformed selected_destination object (i.e., an array instead of a dictionary) does not cause a crash.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@RafaelKayumov RafaelKayumov added this to the 22.7 ❄️ milestone Jul 3, 2025
@RafaelKayumov RafaelKayumov added type: bug A confirmed bug. feature: shipping labels Related to creating, ordering, or printing shipping labels. Bug labels Jul 3, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jul 3, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Number30991
VersionPR #15866
Bundle IDcom.automattic.alpha.woocommerce
Commit0290290
Installation URL0ft87kphon740
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@RafaelKayumov RafaelKayumov requested a review from itsmeichigo July 3, 2025 23:21
Base automatically changed from woomob-752-shipping-labels-update-the-format-for-shipment-id-in to release/22.7 July 4, 2025 03:09
@dangermattic
Copy link
Collaborator

dangermattic commented Jul 4, 2025

1 Warning
⚠️ This PR is assigned to the milestone 22.7 ❄️. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.
1 Message
📖 This PR contains changes to RELEASE-NOTES.txt.
Note that these changes won't affect the final version of the release notes as this version is in code freeze.
Please, get in touch with a release manager if you want to update the final release notes.

Generated by 🚫 Danger

@itsmeichigo itsmeichigo self-assigned this Jul 4, 2025
Copy link
Contributor

@itsmeichigo itsmeichigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this. I tested and confirmed that the fix works with labels created with the correct shipment IDs for hazmat and customs.

The workaround for labels created with invalid shipment IDs didn't seem to work (please see a comment below).

I also see that the origin address is still editable - we would need to use the same solution to parse selected_origin as well. Please feel free to handle that in a separate PR if you prefer.

/// shipment ID to set for hazmat and customs form
var formattedShipmentID: String {
Values.shipmentIDPrefix + shipmentID
return WooShippingShipmentIDFormatter.formattedShipmentID(shipmentID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Values.shipmentIDPrefix is no longer used and should be removed from this file.

}

public extension WooShippingLabelData {
typealias WooShippingLabelDestinations = [String: WooShippingAddress]
Copy link
Contributor

@itsmeichigo itsmeichigo Jul 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Here the destinations are expected to be a dictionary - but for users who created labels with invalid shipment IDs for hazmat and customs, they get an array instead. Then the parsing for selectedDestinations would fail and the workaround in mapDestinations would not be used. We would need to either parse a fallback property as an array to cover that case, or ignore the case entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsmeichigo I didn't implement logic to recover/fallback in case if an array arrives behind selected_destinations. The try?is in the init(from decoder: any Decoder) just to make sure the whole decoding pipeline won't fail. At the moment we're just ignoring selected_destination payload if it's an array.

Regarding the mapDestinations function - the fallback only handles ids without a "shipment_" prefix. I.e. the selected_destination can be a dictionary, but the indexes are just numeric strings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I didn't know there was such case that the destinations are returned as a dictionary with numeric IDs. From my testing, I could only see the array case. Is it reproducible or the workaround in mapDestination is just for safety?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it reproducible

Yep. Check out the Order #1854 in our shared testing store. Addresses there contain both UUID and numeric keys.

// Then
let config = try XCTUnwrap(result.get())
let label = try XCTUnwrap(config.shippingLabelData?.currentOrderLabels.first)
XCTAssertTrue(label.destinationAddress.isEmpty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the workaround in mapDestinations, we should expect destinationAddress to be not empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned here. The successful parse means ignoring the invalid payload and not failing the whole json decoding. The destinationAddress is intended to be empty since we are ignoring the invalid payload.

@RafaelKayumov
Copy link
Contributor Author

I removed the redundant static let shipmentIDPrefix = "shipment_". I'm going to handle the origin address issue in a separate PR. Also I'll implement any other specific fallbacks separately when I figure out more details.

Copy link
Contributor

@itsmeichigo itsmeichigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in a comment here, I cannot reproduce the case with destinations returned as a dictionary with numeric keys. But I left a suggestion for decoding selected destination addresses as an array, it works from my testing. Please consider applying that as a workaround for labels created in the previous version.

Comment on lines +112 to +115
self.selectedDestinations = try? container.decodeIfPresent(
WooShippingLabelDestinations.self,
forKey: CodingKeys.selectedDestination
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Since you already added a workaround for when the addresses are returned as a dictionary with numeric keys, we can add a fallback for the array case here:

Suggested change
self.selectedDestinations = try? container.decodeIfPresent(
WooShippingLabelDestinations.self,
forKey: CodingKeys.selectedDestination
)
self.selectedDestinations = {
do {
let selectedDestinationsArray = try container.decode([WooShippingAddress].self, forKey: .selectedDestination)
var dictionary: [String: WooShippingAddress] = [:]
for (index, item) in selectedDestinationsArray.enumerated() {
dictionary[index.description] = item
}
return dictionary
} catch {
return try? container.decodeIfPresent(
WooShippingLabelDestinations.self,
forKey: CodingKeys.selectedDestination
)
}
}()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented in a separate PR: #15876

@RafaelKayumov RafaelKayumov merged commit 2ad8619 into release/22.7 Jul 8, 2025
13 checks passed
@RafaelKayumov RafaelKayumov deleted the WOOMOB-746-fix-purchased-labels-address-fields branch July 8, 2025 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug feature: shipping labels Related to creating, ordering, or printing shipping labels. type: bug A confirmed bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants